Clean up and simplify Begin/EndInsert #448
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
theory
approved these changes
Nov 13, 2025
Contributor
theory
left a comment
theory
left a comment
There was a problem hiding this comment.
Nice cleanup, especially the elimination of ReceivePreparePackets, thank you! I've confirmed this commit works with the FDW.
|
|
||
| // Send empty block as marker of end of data. | ||
| SendData(Block()); | ||
| inserting = false; |
Contributor
There was a problem hiding this comment.
I had it here because I got an error from the bits waiting for a result when I had it where you've now moved it.
clickhouse/client.cpp
Outdated
| Block Client::Impl::BeginInsert(Query query) { | ||
| if (inserting) { | ||
| if (inserting_) { | ||
| throw ProtocolError("cannot execute query while inserting"); |
Contributor
There was a problem hiding this comment.
Do you want to change this one to ValidationError(), too?
Contributor
Author
There was a problem hiding this comment.
Oops, my bad, yeah, meant to that too!
clickhouse/client.cpp
Outdated
Comment on lines
1146
to
1151
| // Wait for a data packet and return | ||
| uint64_t server_packet = 0; | ||
| while (ReceivePacket(&server_packet)) { | ||
| if (server_packet == ServerCodes::Data) { | ||
| return block; | ||
| } |
Contributor
There was a problem hiding this comment.
So much nicer to remove the duplication, thank you!
- Renamed `InsertData` to `SendInsertBlock`, to prevent it from appearing first when searching for `Insert...` - Removed `EndInsert` with a block. Instead, users should now use `SendInsertBlock` followed by `EndInsert`. - Removed `ReceivePreparePackets` and replaced it with a call to `ReceivePacket`, similar to what is done in `Insert`. - Renamed `inserting` to `inserting_` to follow the member variable naming convention.
slabko
force-pushed
the
begin-end-insert-cleanup
branch
from
9ba3679 to
605a6ba
Compare
Move the `BeginInsert` body to match the order of the class declaration, placing it between `Insert` and `SendInsertData`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
InsertDatatoSendInsertBlock, to prevent it from appearing first when searching forInsert...EndInsertwith a block. Instead, users should now useSendInsertBlockfollowed byEndInsert.ReceivePreparePacketsand replaced it with a call toReceivePacket, similar to what is done inInsert.insertingtoinserting_to follow the member variable naming convention.ProtocolErrorwithValidationErrorfor the situation when calling other operations is not allowed betweenBeginInsertandEndInsertBeginInsertbody to match the order of the class declaration, placing it betweenInsertandSendInsertData.